-
Notifications
You must be signed in to change notification settings - Fork 2
Rjf/omf config and metadata #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: robfitzgerald <7003022+robfitzgerald@users.noreply.github.com>
Co-authored-by: robfitzgerald <7003022+robfitzgerald@users.noreply.github.com>
Co-authored-by: robfitzgerald <7003022+robfitzgerald@users.noreply.github.com>
…earch-bug Fix MaxTripLegs constraint allowing infinite same-mode expansion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds provenance + run-configuration artifacts to OMF network imports (issue #96) by generating a serialized import summary and copying a default BAMBAM config into the import output directory, alongside some multimodal traversal/frontier debugging and a max-trip-legs constraint fix.
Changes:
- Introduce
OmfGraphSummarymetadata/stats and writesummary.tomlduring OMF import. - Copy a default OMF-wired BAMBAM TOML config into the OMF import output directory.
- Fix multimodal
MaxTripLegsconstraint behavior and add a regression test; add debug logging to multimodal components.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/bambam/src/model/traversal/multimodal/model.rs | Adds debug logging around multimodal edge traversal. |
| rust/bambam/src/model/output_plugin/opportunity/source/overture_opportunity_collection_model.rs | Updates collector API usage to pass release URI strings (but needs correct Latest handling). |
| rust/bambam/src/model/label/multimodal/model.rs | Adds debug logging for produced multimodal labels. |
| rust/bambam/src/model/frontier/multimodal/model.rs | Adds debug logging and a new test covering trip-leg-limit behavior. |
| rust/bambam/src/model/frontier/multimodal/constraint.rs | Fixes MaxTripLegs counting logic for same-mode continuation. |
| rust/bambam-omf/src/util/fs.rs | Adds helper to copy default OMF BAMBAM config into output directory. |
| rust/bambam-omf/src/util/bambam-config-omf.toml | Adds default BAMBAM config template intended for OMF-imported directory layout. |
| rust/bambam-omf/src/graph/summary.rs | Introduces OmfGraphSummary + stats aggregation types for import metadata. |
| rust/bambam-omf/src/graph/omf_graph.rs | Writes summary/config during write_compass; adds global avg speed key constant. |
| rust/bambam-omf/src/graph/mod.rs | Exposes new summary types from the graph module. |
| rust/bambam-omf/src/collection/record/transportation_segment.rs | Adds Debug/Clone derives for SegmentFullType. |
| rust/bambam-omf/src/collection/record/transportation_collection.rs | Stores resolved release string on downloaded transportation collections. |
| rust/bambam-omf/src/collection/record/record_type.rs | Changes format_url to accept &str instead of String. |
| rust/bambam-omf/src/collection/collector.rs | Makes get_latest_release public; changes collect_from_release to accept a URI string. |
| rust/bambam-omf/src/app/omf_app.rs | Adds required CLI --name for OMF network import to populate summary metadata. |
| rust/bambam-omf/src/app/network.rs | Builds/writes OmfGraphSummary and passes it into graph writer. |
| rust/bambam-omf/Cargo.toml | Adds toml dependency for summary serialization. |
| configuration/bambam-omf/bambam-config-omf.toml | Adds example config under configuration/ for OMF imports (currently has schema/unit issues). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let uri = self.release_version.to_string(); | ||
| let places_records = self | ||
| .collector | ||
| .collect_from_release( | ||
| self.release_version.clone(), | ||
| &uri, | ||
| &OvertureRecordType::Places, | ||
| self.places_row_filter_config.clone(), |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collect_from_release now expects a fully-resolved release URI (e.g. YYYY-MM-DD.N), but this code passes ReleaseVersion::to_string(). For ReleaseVersion::Latest that becomes the literal string "latest", which likely won’t correspond to a real release path. Resolve Latest via self.collector.get_latest_release() (now public) and only use ReleaseVersion string formatting for non-latest releases.
| frontier.type = "combined" | ||
| frontier.models = [ | ||
| { type = "time_limit", time_limit = { time = 40.0, time_unit = "minutes" }}, | ||
| { type = "multimodal", mode = "drive", constraints = [], available_modes = ["walk", "bike", "drive"], available_route_ids = [], use_route_ids = false, max_trip_legs = 5 } | ||
| ] |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deserialization issue here: the multimodal frontier model config key should be this_mode, not mode, to match MultimodalFrontierConfig.
| pub struct OmfGraphStats { | ||
| /// number of vertices in the network | ||
| pub vertices: usize, | ||
| /// details for each edge list | ||
| pub edge_list: HashMap<String, EdgeListStats>, | ||
| } |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OmfGraphSummary is serialized to TOML for provenance/recordkeeping; using HashMap for edge_list means the key order in summary.toml is nondeterministic between runs (HashMap iteration order is randomized). If stable diffs are desired, consider using BTreeMap for edge_list and road_class_stats so output ordering is deterministic.
| /// copies bambam-config-omf.toml to the directory of an OMF import. | ||
| pub fn copy_default_config(output_directory: &Path) -> Result<(), OvertureMapsCollectionError> { | ||
| let src = PathBuf::from(env!("CARGO_MANIFEST_DIR")) | ||
| .join("src") | ||
| .join("util") | ||
| .join("bambam-config-omf.toml"); |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy_default_config reads from env!("CARGO_MANIFEST_DIR")/src/... at runtime, which will fail for installed/distributed binaries (where the crate source isn’t present). To make this reliable, embed the default TOML in the binary (e.g. include_str!/include_bytes!) and write it to the output directory, or otherwise package it as a runtime asset.
| traversal.type = "combined" | ||
| traversal.models = [ | ||
| { type = "distance", distance_unit = "miles" }, | ||
| { type = "speed", name = "drive", speed_unit = "kph", speed_table_input_file = "drive/edges-speeds-mph-enumerated.txt.gz" }, |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The drive speed table filename indicates MPH (edges-speeds-mph-enumerated.txt.gz), and the OMF import code computes/serializes speeds in mph (see segment_split.rs using mile_per_hour). Setting speed_unit = "kph" here will misinterpret the table values during routing. Update the unit to mph (or rename/regenerate the table to match kph).
| { type = "speed", name = "drive", speed_unit = "kph", speed_table_input_file = "drive/edges-speeds-mph-enumerated.txt.gz" }, | |
| { type = "speed", name = "drive", speed_unit = "mph", speed_table_input_file = "drive/edges-speeds-mph-enumerated.txt.gz" }, |
|
|
||
| // summarize imported graph | ||
| let release = match local_source { | ||
| Some(local) => format!("file://{}", local.to_str().unwrap_or_default()), |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For local imports, this constructs a file://... URI using Path::to_str().unwrap_or_default(). This can produce an invalid/empty URI for non-UTF8 paths, and file:// (two slashes) is typically not a valid absolute file URI (usually file:///...). Consider using to_string_lossy() and emitting either a plain path or a properly formed file URI.
| Some(local) => format!("file://{}", local.to_str().unwrap_or_default()), | |
| Some(local) => format!("file://{}", local.to_string_lossy()), |
| frontier.type = "combined" | ||
| frontier.models = [ | ||
| { type = "time_limit", time_limit = { time = 40.0, time_unit = "minutes" }}, | ||
| { type = "multimodal", mode = "bike", constraints = [], available_modes = ["walk", "bike", "drive"], available_route_ids = [], use_route_ids = false, max_trip_legs = 5 } | ||
| ] |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same deserialization issue here: the multimodal frontier model config key should be this_mode, not mode, to match MultimodalFrontierConfig in rust/bambam/src/model/frontier/multimodal/config.rs.
| frontier.type = "combined" | ||
| frontier.models = [ | ||
| { type = "time_limit", time_limit = { time = 40.0, time_unit = "minutes" }}, | ||
| { type = "multimodal", this_mode = "walk", constraints = [], available_modes = ["walk", "bike", "drive"], available_route_ids = [], use_route_ids = false, max_trip_legs = 5 } | ||
| ] |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The multimodal frontier model config includes available_route_ids and use_route_ids, but MultimodalFrontierConfig expects route_ids_input_file and does not have a use_route_ids field. These unknown keys will fail deserialization; remove them and use route_ids_input_file if route IDs are needed (also repeated in other mode sections).
| { type = "distance", distance_unit = "miles" }, | ||
| { type = "fixed_speed", name = "walk", speed = 5.0, speed_unit = "kph" }, | ||
| { type = "time", time_unit = "minutes" }, | ||
| { type = "multimodal", this_mode = "walk", available_modes = ["walk", "bike", "drive"], available_route_ids = [], use_route_ids = false, max_trip_legs = 5 } | ||
| ] |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This multimodal traversal config includes available_route_ids and use_route_ids, but the traversal config struct uses route_ids_input_file and does not accept these fields. As written, this TOML will fail to deserialize into the traversal model config. Remove these keys and use route_ids_input_file (or omit it) instead; same issue repeats in the bike/drive sections.
| frontier.type = "combined" | ||
| frontier.models = [ | ||
| { type = "time_limit", time_limit = { time = 40.0, time_unit = "minutes" }}, | ||
| { type = "multimodal", mode = "walk", constraints = [], available_modes = ["walk", "bike", "drive"], available_route_ids = [], use_route_ids = false, max_trip_legs = 5 } |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue for the multimodal frontier config: available_route_ids/use_route_ids aren’t fields on MultimodalFrontierConfig (it uses route_ids_input_file). These unknown keys will cause deserialization errors; remove them and use route_ids_input_file if needed.
| { type = "multimodal", mode = "walk", constraints = [], available_modes = ["walk", "bike", "drive"], available_route_ids = [], use_route_ids = false, max_trip_legs = 5 } | |
| { type = "multimodal", mode = "walk", constraints = [], available_modes = ["walk", "bike", "drive"], max_trip_legs = 5 } |
this PR Closes #96 by introducing an OmfGraphSummary struct which collects metadata about the import and resulting graph. the summary is written to disk at the output directory of an OMF import with file contents that look like this. additionally, a default configuration file located at rust/bambam-omf/src/util/bambam-config-omf.toml is copied into the output directory. this working bambam configuration is wired for the directory and file layout of an OMF import and has placeholder entries for other configurable features of a BAMBAM run.
my test command has been
which when run from the repo root, writes to the folder denver_co/.
note: running bambam on the result does not succeed due to unrelated issues that are out of scope for this PR. i'm seeing this error related to max_trip_len off-by-one errors: